feat: Add support for wrapping the devtools APIs #57

rpl · 2017-09-23T13:07:27Z

This PR is applying the following changes to fix #56:

the changes into api-metadata.json, needed to instruct the polyfill to wrap the devtools API namespace)
a fix in the base browser-polyfill.js module to prevent the TypeError exception when the getter on the proxy is related to a non-configurable read-only property of the original Proxy target (fixed by using an empty object which has the original target as its prototype as the "root" target of the Proxy instance created by the polyfill)
a fix in the base browser-polyfill.js module to force devtools.panels.create to return a promise that resolves to a single callback argument (instead of an resolving to an array of arguments which contains an additional undocumented second parameter)
fix applied on one of the existent tests (related to the Proxy TypeError fix)
2 additional test case (which covers the Proxy TypeError fix and the new maxResolvedArgs property in the devtools.panels.create metadata).

rpl · 2017-09-23T13:10:11Z

Hi @Rob--W
do you mind to take a look at this PR?

Rob--W · 2017-09-23T15:03:27Z

src/browser-polyfill.js

+          if ("maxResolvedArgs" in metadata) {
+            // Some chrome APIs provides to their callback more arguments than expected.
+            callbackArgs = callbackArgs.slice(0, metadata.maxResolvedArgs);
+          }


There is no need to be so generic. Since the problem only occurs when with 1 vs not-one parameter, I suggest to throw away "maxResolvedArgs" and use "singleCallbackParameter": true or something similar. This could also be used to fix API definitions for callbacks that don't expect any parameter. Based on the code below, it is likely that callbacks without parameters resolve to an empty array.

Rob--W · 2017-09-23T15:04:09Z

test/test-async-functions.js

+          panels: {
+            create: sinon.spy((title, iconPath, panelURL, cb) => {
+              Promise.resolve().then(() => {
+                cb({isFakePanel: true}, "unwanted parameter");


Leave a comment what happens with "unwanted parameter", and why.

Rob--W · 2017-09-23T15:05:52Z

test/test-proxied-properties.js

@@ -75,28 +105,28 @@ describe("browser-polyfill", () => {
    });

    it("deletes proxy getter/setter that are not wrapped", () => {
-      const fakeChrome = {};
+      const fakeChrome = {runtime: {}};


Why these changes? Please explain the changes in the commit description.

Rob--W · 2017-09-23T15:18:36Z

I have filed an upstream bug for Chrome's misbehaving devtools.panels.create API: https://bugs.chromium.org/p/chromium/issues/detail?id=768159

rpl · 2017-09-25T16:54:39Z

@Rob--W Thanks a lot for the review comments (and for filing the issue on Chromium issue tracker), I've applied the following changes based on you review comments:

changed the new metadata property to singleCallbackArg, and added a test case related to a callback which expects 1 parameter called without any parameter (so that we test explicitly that the promise will resolve to a single undefined parameter instead of an empty array of callback parameters), and added to the test cases an inline comment that explains what behavior we expect
squashed all the commits upfront and added a commit description comment which explains the changes applied (which included some additional information about the reason of the changes applied to the existent test case related to the "proxy setter/getter behaviors on non wrapped properties"): 676966d
tests

…to wrapped APIs. To be able to wrap the devtools API namespaces, this patch applies the following changes: - fix: Prevent Proxy violation exception on the read-only/non-configurable devtools property by using an empty object with the `chrome` API object as its prototype as the root Proxy target (the Proxy instances returned for the `chrome` API object) and add a related test case. - fix: Added support for a new `singleCallbackArg` metadata property, which prevents devtools.panels.create to resolve an array of parameters (See the related Chromium issue at https://bugs.chromium.org/p/chromium/issues/detail?id=768159) and add the related test cases. - test: Changes to the test case related to proxy getter/setter behavior on non wrapped properties: in the "deletes proxy getter/setter that are not wrapped" test case from the "test-proxied-properties.js" test file, we ensure that when a getter/setter is called for a "non-wrapped" property, the getter/setter is going to affect the original target object, unfortunately this in not true anymore for the root object (the `chrome` API object) because we are using an empty object (which has the `chrome` API object as its prototype and it is not exposed outside of the polyfill sources) as the target of the Proxy instance related to it, this change to the target of the Proxy has been needed to prevent the TypeError exception raised by the Proxy instance when we try to access the "devtools" property (which is non-configurable and read-only on the `chrome` API object).

marcysutton · 2017-09-25T21:45:39Z

Thank you so much for this! Excited to give it a go. Will report back in a new issue if I run into any problems.

marcysutton · 2017-09-29T01:24:33Z

Do you know when this will get released? I'm not seeing it in 0.1.2. Thanks!

rpl · 2017-09-29T12:49:13Z

@marcysutton I'm going to release a new version today (I'd like to include also #59, but if we are not able to land it today yet, I will release a new version for this change and I will include #59 in the next one).

Originally, the polyfill created a Proxy with the original API object as the target. This was changed to `Object.create(chrome)` because not doing so would prevent the `browser.devtools` API from working because the devtools API object is assigned as a read-only & non-configurable property (mozilla#57). However, that action itself caused a new bug: Whenever an API object is dereferenced via the `browser` namespace, the original API is no longer available in the `chrome` namespace, and trying to access the API through `chrome` returns `undefined` plus the "Previous API instantiation failed" warning (mozilla#58). This is because Chrome lazily initializes fields in the `chrome` API, but on the object from which the property is accessed, while the polyfill accessed the property through an object with the prototype set to `chrome` instead of directly via chrome. To fix that, `Object.create(chrome)` was replaced with `Object.assign({}, chrome)`. This fixes both of the previous issues because 1) It is still a new object. 2) All lazily initialized fields are explicitly initialized. This fix created a new issue: In Chrome some APIs cannot be used even though they are visible in the API (e.g. `chrome.clipboard`), so calling `Object.assign({}, chrome)` causes an error to be printed to the console (mozilla#70). To solve this, I use `Object.create(chrome)` again as a proxy target, but dereference the API via the original target (`chrome`) to not regress on mozilla#58. Besides fixing the bug, this also reduces the performance impact of the API because all API fields are lazily initialized again, instead of upon start-up. This fixes mozilla#70.

Originally, the polyfill created a Proxy with the original API object as the target. This was changed to `Object.create(chrome)` because not doing so would prevent the `browser.devtools` API from working because the devtools API object is assigned as a read-only & non-configurable property (#57). However, that action itself caused a new bug: Whenever an API object is dereferenced via the `browser` namespace, the original API is no longer available in the `chrome` namespace, and trying to access the API through `chrome` returns `undefined` plus the "Previous API instantiation failed" warning (#58). This is because Chrome lazily initializes fields in the `chrome` API, but on the object from which the property is accessed, while the polyfill accessed the property through an object with the prototype set to `chrome` instead of directly via chrome. To fix that, `Object.create(chrome)` was replaced with `Object.assign({}, chrome)`. This fixes both of the previous issues because 1) It is still a new object. 2) All lazily initialized fields are explicitly initialized. This fix created a new issue: In Chrome some APIs cannot be used even though they are visible in the API (e.g. `chrome.clipboard`), so calling `Object.assign({}, chrome)` causes an error to be printed to the console (#70). To solve this, I use `Object.create(chrome)` again as a proxy target, but dereference the API via the original target (`chrome`) to not regress on #58. Besides fixing the bug, this also reduces the performance impact of the API because all API fields are lazily initialized again, instead of upon start-up. This fixes #70.

rpl requested a review from Rob--W September 23, 2017 13:08

Rob--W reviewed Sep 23, 2017

View reviewed changes

rpl force-pushed the fix/56-add-missing-devtools-api branch from 97695a3 to 676966d Compare September 25, 2017 16:27

rpl force-pushed the fix/56-add-missing-devtools-api branch from 676966d to d343d0c Compare September 25, 2017 18:12

Rob--W approved these changes Sep 25, 2017

View reviewed changes

rpl added component: api metadata component: polyfill wrappers labels Sep 25, 2017

rpl force-pushed the fix/56-add-missing-devtools-api branch from d343d0c to 6dc0c38 Compare September 25, 2017 20:11

rpl changed the title ~~fix: Add support for wrapping the devtools APIs~~ feat: Add support for wrapping the devtools APIs Sep 25, 2017

rpl merged commit 61d92ea into mozilla:master Sep 25, 2017

rpl mentioned this pull request Sep 26, 2017

Fix: Support pageAction show/hide wrapping #59

Merged

Rob--W mentioned this pull request Oct 2, 2017

runtime.sendMessage/onMessage #58

Closed

This was referenced Oct 9, 2017

fix: Prevent 'Previous API instantiation failed' errors on runtime.sendMessage API calls #64

Merged

runtime namespace becomes undefined after certain sequence of requests #65

Closed

rpl deleted the fix/56-add-missing-devtools-api branch March 15, 2021 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for wrapping the devtools APIs #57

feat: Add support for wrapping the devtools APIs #57

rpl commented Sep 23, 2017

rpl commented Sep 23, 2017

Rob--W Sep 23, 2017

Rob--W Sep 23, 2017

Rob--W Sep 23, 2017

Rob--W commented Sep 23, 2017

rpl commented Sep 25, 2017

marcysutton commented Sep 25, 2017

marcysutton commented Sep 29, 2017

rpl commented Sep 29, 2017

feat: Add support for wrapping the devtools APIs #57

feat: Add support for wrapping the devtools APIs #57

Conversation

rpl commented Sep 23, 2017

rpl commented Sep 23, 2017

Rob--W Sep 23, 2017

Choose a reason for hiding this comment

Rob--W Sep 23, 2017

Choose a reason for hiding this comment

Rob--W Sep 23, 2017

Choose a reason for hiding this comment

Rob--W commented Sep 23, 2017

rpl commented Sep 25, 2017

marcysutton commented Sep 25, 2017

marcysutton commented Sep 29, 2017

rpl commented Sep 29, 2017